Add no-ignored-test-files rule (fixes #51)#56
Conversation
ea68124 to
a547b54
Compare
The watcher does this, see https://github.com/sindresorhus/ava/blob/005b656b9950ab3fa1e2bd9af326d5b39540e71b/lib/watcher.js#L333:L379. It uses https://www.npmjs.com/package/multimatch. |
|
From what I could test, in Atom's ESLint plugin (the Atom plugin that runs ESLint I mean), So it means two things:
The second is obviously pretty rare (I'd think at least), but it can be problematic for some users. Let me know what you guys think about this. |
|
@jfmengels could use https://www.npmjs.com/package/pkg-up until you find a package that depends on AVA? |
|
@novemberborn Sounded good, so I tried it and it works well. I disabled the rule altogether progammatically when no Haven't squashed the commits so that I could revert back to previous impl, as someone might find it unacceptable to depend on a package.json file. I'll squash and merge when people are happy with this. |
rules/no-ignored-test-files.js
Outdated
| ignoredFolder = 'helpers'; | ||
| } | ||
|
|
||
| if (ignoredFolder) { |
There was a problem hiding this comment.
Why not use multimatch here as well? Copy excludePatterns without the leading ! and you should be good to go.
There was a problem hiding this comment.
Sounds good! Will do that (won't put node_modules though as ESLint ignores them already)
|
Do people put test factories in their helpers? That might be a legitimate Very cool @jfmengels! |
|
@novemberborn @jamestalmage had the same comment #51 (comment), and I agree that you should then simply disable the rule. There was an interesting proposition avajs/ava#695 for test macros which would be better. We only lint an error if you import AVA and create a test, but we could then make an exception if you're defining a macro |
b25192e to
a4b65ae
Compare
|
Updated :) |
That sounds good. |
docs/rules/no-ignored-test-files.md
Outdated
| @@ -0,0 +1,92 @@ | |||
| # Ensure no tests are written in ignored files | |||
|
|
|||
| When searching for tests, AVA ignores files contained in folders named `fixtures` or `helpers`. By default, it will search in `test.js test-*.js test/**/*.js`, which you can override by specifying a path when launching AVA or in the [AVA configuration in the `package.json` file] ](https://github.com/sindresorhus/ava#configuration). | |||
There was a problem hiding this comment.
Well, ESLint ignores node_modules anyway. I don't really mind adding it, but for consistency we'd have to add it to every rule. I think that users of Node or ESLint know that node_modules/ should not be touched.
There was a problem hiding this comment.
Yes but you're describing AVA's behavior here.
There was a problem hiding this comment.
True. I've added the mention about node_modules, and to clarify, a message saying that they won't be linted due to ESLint.
|
Nothing further from me 👯 |
e1ff1d3 to
46dca09
Compare
|
LGTM |
|
Cool, I'll squash and merge then. |
46dca09 to
b0da32c
Compare
|
@jfmengels - FYI - GH has a new "Squash and Merge" feature built in. Just click the merge button and you will see a drop down that gives you that option. Ideally you would use that, even for 1 commit PR's. It avoids creating an extra merge commit. If there are multiple commits that you want to keep separate, the old merge method is fine. |
|
Aah. Yeah I knew about it, but I thought it was something that you needed to activate as I didn't see it. But it only shows when you click the merge button once apparently ^^' (Hoping for a rebase button to come soon) |
|
You usually only need to rebase if there is a merge conflict (in which case you probably want to do some edits / testing locally). I guess it could be useful for selectively squashing (i.e. ending up with less commits, but still more than one), and for editing commit messages. But that is a rare enough need I'm not clamoring for it. I would much rather see improvements to issue management first. |
|
I mean merge rebase in the sense of no merge commits. I like straight lines. |
That's exactly what the squash button does, but you seems you used the old merge button. |

Add no-ignored-test-files rule (fixes #51)
I would like to add more checks to see whether the test is in the AVA glob (with an option to de-/activate or set a custom glob), but I haven't found (yet, haven't searched much for now) a library that simply tells me whether a file matches a glob, and I don't know if we can reliably get the cwd from ESLint (or the editor running ESLint).
This can be added later to the rule though. I might take a look at it later, or someone could take a look too if interested.